fix(opencode): surface truncated turns instead of ending the loop - #40142
Open
iceteaSA wants to merge 1 commit into
Open
fix(opencode): surface truncated turns instead of ending the loop#40142iceteaSA wants to merge 1 commit into
iceteaSA wants to merge 1 commit into
Conversation
Contributor
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
Contributor
|
The following comment was made by an LLM, it may be inaccurate: One related PR was found: PR #26167: fix(session): retry empty stream truncations and discard partial parts This PR appears related as it addresses truncation handling in sessions, specifically dealing with empty stream truncations and partial parts. While the scope differs from PR #40142 (which focuses on surfacing truncated turns by checking |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue for this PR
Fixes #40146.
That issue is item 4 of the root-cause chain in #18108 (P1 on its checklist), filed separately because #18108 covers five interacting problems and this one is independently fixable.
Related: #38747 (V2-side truncated-stream recovery — same problem class, different layer), #29363 (the 32K
OUTPUT_TOKEN_MAXclamp that makes truncation common — deliberately out of scope here).Type of change
What does this PR do?
A turn that finishes
lengthwas classified as a normal completion. Two checks in the session loop exclude only"tool-calls"(and"unknown"), never"length", so a provider that truncated its output at the token limit ended the session exactly as though the model had chosen to stop.That is #18108's P1, filed in March:
The failure is worst when the truncated turn produced nothing usable. A subagent on an OpenAI-compatible gateway spent its entire output budget inside the reasoning channel — parts were
step-start+reasoning(129,961 chars) +step-finish, no text and no tool call,finish: "length"— and the task completed with statusokand empty output. From the caller's side that is indistinguishable from a subagent that had nothing to say. Nothing anywhere insrcbranched on"length".The change: when a turn finishes
length, branch on whether it produced anything worth continuing from.lengthOutputLengthError, stoplengthOutputLengthError, stopThree decisions worth explaining, since each has a plausible-looking alternative:
Continuation is bounded to exactly one attempt, with no counter. The bound reads
lastAssistant.finishfrom history the loop already reloads each iteration.agent.stepscould not be reused for this: it defaults toInfinity, and at the limit it only injectsMAX_STEPS_PROMPTrather than breaking — it is a nudge, not a bound. An unbounded continuation would be worse than the original bug, since each iteration costs a full output budget.Recoverability is decided by part presence, not token counts. Some gateways report reasoning inside the ordinary output count, so
tokens.reasoningreads0on exactly the turns this targets. A token-based check would be vacuous where it matters most.OutputLengthErroris reused, not newly defined. It already exists in the schema, in core, insession/message-error.ts, is already a member of the assistant error union, andacp/service.tsalready maps it tostopReason: "max_tokens". It had no producer anywhere insrc— the plumbing was built for this case and never connected. This connects it.The entry-gate change is load-bearing rather than cosmetic: the loop reloads history at the top of every iteration, so without excluding
"length"there, the reloaded state (a truncated message with no tool calls) exits before the continuation turn can run. A reviewer confirmed this independently by reverting that hunk alone.One behavioural note for reviewers: the entry gate also governs direct
loop()callers. A session whose last assistant turn finishedlengthand is resumed through thesummarizeendpoint will now take an additional provider turn where it previously stopped. For that path the caller has explicitly asked for compaction and resumption, so continuing seems right — but it is a real change and worth a second opinion.The 32K
OUTPUT_TOKEN_MAXclamp that makes truncation common in the first place is not touched here. Four PRs against that constant have been closed without merging; it is contested design territory and belongs in its own change. This PR is about not misreporting truncation when it happens.How did you verify your code works?
Three integration tests in
packages/opencode/test/session/prompt.test.ts, plus alength()finish on the test LLM server's reply builder. All three were written first and confirmed failing (0 pass / 3 fail) before any production edit.lengthturn with text, followed by astopturn — asserts exactly two provider requests and that the final message carries the completed text.MessageOutputLengthErroron both the returned and the persisted message, with the reasoning part preserved.lengthturns with a sentinel third reply queued — asserts exactly two provider requests, so an unbounded implementation fails an assertion rather than hanging.Mutation-checked in both directions: reverting the production hunks turns all three red, restoring returns them green. A cross-family reviewer independently reverted each of the two hunks separately and confirmed the bound is what makes the third test pass — with the in-loop hunk alone reverted, the loop runs three provider turns instead of two.
bun testinpackages/opencode: 3231 pass / 0 fail (baseline ondevis 3228).bun typecheckclean inpackages/opencodeandpackages/core.One honest limitation: the first test does not discriminate the in-loop hunk on its own — with that hunk reverted,
SessionProcessor.processreturns"continue"anyway and the observable outcome is identical. It documents end-to-end recovery; the in-loop logic is pinned by the other two tests.Screenshots / recordings
Not a UI change.
Checklist